Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for GDB JIT debugging #1212

Merged
merged 29 commits into from
Feb 27, 2020
Merged

Add support for GDB JIT debugging #1212

merged 29 commits into from
Feb 27, 2020

Conversation

MarkMcCaskey
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey commented Feb 13, 2020

This PR adds support for JIT debugging to Wasmer with the Cranelift backend using a fork of wasmtime-debug.

The motivation for this change is partially inspired by the feature in Wasmtime and the implementation is largely derived from Wasmtime's wasmtime-debug crate and not included in this PR. This implementation is currently Cranelift-only (if LLVM has value tracking we can add this there too without too much effort; we'd have to do the value tracking ourselves in Singlepass and I don't have enough context to know how hard that would be) and is based on a generic fork of the wasmtime-debug -- which will be published and uploaded in another repo.

This PR started out implementing the Wasm-DWARF reading and writing with gimli but after working on it for a few days, reading a chunk of the DWARF spec, seeing that Wasmtime had solved this well, and realizing how long this would likely take, I decided that it didn't make sense to spend the engineering effort there so I made a copy of wasmtime-debug and removed some of the less portable Cranelift pieces (very minor changes) and all code relying on data structures from wasmtime. The resulting crate is completely generic and would work fine with Wasmtime or any other Wasm runtime at the cost of requiring some transmutes or a linear pass over the debug data to reconstruct it in terms of the new types exposed by the fork. Perhaps there's a cleaner way to handle that that I haven't considered.

The integration with the GDB JIT interface is from the LLVM examples (I don't remember if I properly attributed everything in this PR/version of the code -- I still have the other branches locally though which I'll review before shipping this) and some of the code in this PR is from Wasmtime/Cranelift source code such as the sorting of the ebbs in clif_backend::resolver. I spent a long time debugging some subtle bugs and ended up using a few things from Wasmtime's integration with wasmtime-debug and some bits from cranelift-wasm and cranelift-codegen.

If there's interest from other people in working on the generic wasmtime-debug fork, I'm happy to get other maintainers involved and/or move it to a shared organization.

Special thanks to Yury Delendik and the other wasmtime-debug authors for their work on Wasm debugging. Also shout out to Cranelift for the nice API for tracking variables/data.

TODO:

  • Update attributions file for LLVM, wasm-dwarf, and Wasmtime/Cranelift and do another pass over code to make sure we're in compliance with the licenses from the relevant projects and have properly attributed the code used from other projects.
  • Adjust API of wasm-debug based on feedback
  • Discuss with Nick integration with LLVM
  • Discuss with Heyang integration with Singlepass
  • Adjust implementation based on feedback from team (traits modified, etc.)
  • Clean up some pointer wrangling code
  • Add opt-in feature to wasmer-runtime-core to enale wasm-debug so library users who won't use debug info are not affected

Review

  • Add a short description of the the change to the CHANGELOG.md file

@MarkMcCaskey MarkMcCaskey added 📦 lib-cli About wasmer-cli 📦 lib-compiler-cranelift About wasmer-compiler-cranelift 📦 lib-deprecated About the deprecated crates labels Feb 14, 2020
lib/clif-backend/src/code.rs Outdated Show resolved Hide resolved
lib/clif-backend/src/code.rs Outdated Show resolved Hide resolved
lib/clif-backend/src/code.rs Outdated Show resolved Hide resolved
lib/clif-backend/src/resolver.rs Outdated Show resolved Hide resolved
lib/runtime-core/src/instance.rs Outdated Show resolved Hide resolved
lib/runtime-core/src/jit_debug.rs Outdated Show resolved Hide resolved
lib/runtime-core/src/jit_debug.rs Outdated Show resolved Hide resolved
@@ -78,6 +78,11 @@ where
pub fn into_vec(self) -> Vec<V> {
self.elems
}

/// Iterate over the values of the map in order
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Iterate over the values of the map in order
/// Iterate over the values of the map in order.

lib/runtime-core/src/vm.rs Outdated Show resolved Hide resolved
lib/runtime-core/src/vm.rs Outdated Show resolved Hide resolved
Mark McCaskey and others added 2 commits February 20, 2020 11:20
@@ -296,6 +364,7 @@ impl MiddlewareChain {
fcg: Option<&mut FCG>,
ev: Event,
module_info: &ModuleInfo,
loc: u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we changing the public API? If so, is there any way to either:

  • Have this added into the CHANGELOG as [BREAKING CHANGE]
  • Minimize this? (not sure how, we will have to research)

Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, thanks!

I left few notes. In jit_debug.rs, sometimes we use JITFoo, sometimes JitFoo. We should make it uniform if you think it is worth the time.

I didn't test the code. I only made a review.

ATTRIBUTIONS.md Outdated
of generating debug information for each function with Cranelift
(for example, the sorting of the extended basic blocks before
processing the instructions), and the API for transforming DWARF
see wasm-debug's attribution file for more information (TODO: link
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we address the TODO before the merge?

CHANGELOG.md Outdated
@@ -6,6 +6,7 @@

- [#1233](https://github.com/wasmerio/wasmer/pull/1233) Improved Wasmer C API release artifacts.
- [#1216](https://github.com/wasmerio/wasmer/pull/1216) `wasmer-interface-types` receives a binary encoder.
- [#1212](https://github.com/wasmerio/wasmer/pull/1212) Add `--generate-debug-info` and `-g` flags to `wasmer run` to generate debug information during compilation that is passed via the GDB JIT interface to a debugger to allow source-level debugging of Wasm files. Currently only available on clif-backend, see PR for more information on its implementation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two spaces before “Currently”:

Suggested change
- [#1212](https://github.com/wasmerio/wasmer/pull/1212) Add `--generate-debug-info` and `-g` flags to `wasmer run` to generate debug information during compilation that is passed via the GDB JIT interface to a debugger to allow source-level debugging of Wasm files. Currently only available on clif-backend, see PR for more information on its implementation.
- [#1212](https://github.com/wasmerio/wasmer/pull/1212) Add `--generate-debug-info` and `-g` flags to `wasmer run` to generate debug information during compilation that is passed via the GDB JIT interface to a debugger to allow source-level debugging of Wasm files. Currently only available on clif-backend, see PR for more information on its implementation.

Also, should we adopt the classic compiler -O option? -O0, -O1, -O2, -Oz etc.? I'll open a new issue to discuss about that. I'm even thinking about a -C option.

Please read #1237 before merging this PR 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a big fan of two spaces after periods. I try to suppress it when writing in a shared space because inconsistent style is worse, but I find it much more readable, it chunks the sentences in my brain better and helps me read more quickly.

From Googling it, using two spaces after a period apparently means I'm over 40 years old and stuck to my ways using the typewriter 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's quite a few instances of \. in CHANGELOG.md haha, probably all from me

@@ -71,6 +71,7 @@ impl ModuleCodeGenerator<CraneliftFunctionCodeGenerator, Caller, CodegenError>
fn next_function(
&mut self,
module_info: Arc<RwLock<ModuleInfo>>,
loc: (u32, u32),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we use .0 and .1 more than once in this code, can we imagine a LocationSpan structure, with the start and end fields?

/// Data about the the compiled machine code.
type CompileMetadata = (
LocalFuncIndex,
Option<(CompiledFunctionData, ValueLabelsRanges, Vec<Option<i32>>)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i32 in Vec<Option<i32>> refers to locations? If yes, why not u32 instead?

Also, maybe it is interesting to add type Location = u32. I think it would clarify the code for such complex type. Thoughts?

Copy link
Contributor Author

@MarkMcCaskey MarkMcCaskey Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, it came from the stackslot type that wasm-debug used to depend on. The offset it returns is a number. I believe they're usually negative numbers actually because they're in terms of RBP and not RSP 🤔

@@ -0,0 +1,195 @@
//! Code for interacting with the
//! [GDB JIT inteface](https://sourceware.org/gdb/current/onlinedocs/gdb.html#JIT-Interface).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! [GDB JIT inteface](https://sourceware.org/gdb/current/onlinedocs/gdb.html#JIT-Interface).
//! [GDB JIT interface](https://sourceware.org/gdb/current/onlinedocs/gdb.html#JIT-Interface).

/// Node of the doubly linked list that the GDB JIT interface reads from.
#[no_mangle]
#[repr(C)]
pub(crate) struct JITCodeEntry {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we want JITCodeEntry to be public in the crate? I don't see any reason. Am I missing something?

Suggested change
pub(crate) struct JITCodeEntry {
struct JITCodeEntry {

/// Head node of the doubly linked list that the GDB JIT interface expects.
#[no_mangle]
#[repr(C)]
pub(crate) struct JitDebugDescriptor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we want JitDebugDescriptor to be public in the crate? I don't see any reason. Am I missing something?

Suggested change
pub(crate) struct JitDebugDescriptor {
struct JitDebugDescriptor {

/// debugger to perform.
#[no_mangle]
#[allow(non_upper_case_globals)]
pub(crate) static mut __jit_debug_descriptor: JitDebugDescriptor = JitDebugDescriptor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we want __jit_debug_descriptor to be public in the crate? I don't see any reason. Am I missing something?

Suggested change
pub(crate) static mut __jit_debug_descriptor: JitDebugDescriptor = JitDebugDescriptor {
static mut __jit_debug_descriptor: JitDebugDescriptor = JitDebugDescriptor {


/// Manager of debug info registered with the debugger.
#[derive(Debug, Clone, Default)]
pub struct JITCodeDebugInfoManager {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub struct JITCodeDebugInfoManager {
pub(crate) struct JITCodeDebugInfoManager {

since there is only one field, which is private, and one method, which is visible to the crate only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's part of module which has all public fields. I wasn't sure if I should make the first private field in the struct or just make it public. Making it public seemed like the safer choice

#[cfg(feature = "generate-debug-information")]
#[serde(skip)]
/// Resource manager of debug information being used by a debugger.
pub debug_info_manager: jit_debug::JITCodeDebugInfoManager,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub debug_info_manager: jit_debug::JITCodeDebugInfoManager,
pub(crate) debug_info_manager: jit_debug::JITCodeDebugInfoManager,

related to my comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit worried about the implications of making the first non-public field, but I suppose it'll be fine

Co-authored-by: Ivan Enderlin <[email protected]>
bors bot added a commit that referenced this pull request Feb 27, 2020
1212: Add support for GDB JIT debugging r=MarkMcCaskey a=MarkMcCaskey

This PR adds support for JIT debugging to Wasmer with the Cranelift backend using a fork of `wasmtime-debug`.

The motivation for this change is partially inspired by the feature in Wasmtime and the implementation is largely derived from Wasmtime's `wasmtime-debug` crate and not included in this PR.  This implementation is currently Cranelift-only (if LLVM has value tracking we can add this there too without too much effort; we'd have to do the value tracking ourselves in Singlepass and I don't have enough context to know how hard that would be) and is based on a generic fork of the `wasmtime-debug` -- which will be published and uploaded in another repo.

This PR started out implementing the [Wasm-DWARF](https://yurydelendik.github.io/webassembly-dwarf/) reading and writing with gimli but after working on it for a few days, reading a chunk of the DWARF spec, seeing that Wasmtime had solved this well, and realizing how long this would likely take, I decided that it didn't make sense to spend the engineering effort there so I made a copy of `wasmtime-debug` and removed some of the less portable Cranelift pieces (very minor changes) and all code relying on data structures from wasmtime.  The resulting crate is completely generic and would work fine with Wasmtime or any other Wasm runtime at the cost of requiring some `transmute`s or a linear pass over the debug data to reconstruct it in terms of the new types exposed by the fork.  Perhaps there's a cleaner way to handle that that I haven't considered.

The integration with the GDB JIT interface is from the LLVM examples (I don't remember if I properly attributed everything in this PR/version of the code -- I still have the other branches locally though which I'll review before shipping this) and some of the code in this PR is from Wasmtime/Cranelift source code such as the sorting of the `ebb`s in `clif_backend::resolver`.  I spent a long time debugging some subtle bugs and ended up using a few things from Wasmtime's integration with `wasmtime-debug` and some bits from `cranelift-wasm` and `cranelift-codegen`.

If there's interest from other people in working on the generic `wasmtime-debug` fork, I'm happy to get other maintainers involved and/or move it to a shared organization.

Special thanks to [Yury Delendik](https://github.com/yurydelendik) and the other `wasmtime-debug` authors for their work on Wasm debugging.  Also shout out to Cranelift for the nice API for tracking variables/data.

### TODO:
- [x] Update attributions file for LLVM, [wasm-dwarf](https://github.com/yurydelendik/wasm-dwarf), and Wasmtime/Cranelift and do another pass over code to make sure we're in compliance with the licenses from the relevant projects and have properly attributed the code used from other projects.
- [x] Adjust API of wasm-debug based on feedback
- [x] Discuss with Nick integration with LLVM
- [x] Discuss with Heyang integration with Singlepass
- [x] Adjust implementation based on feedback from team (traits modified, etc.)
- [x] Clean up some pointer wrangling code
- [x] Add opt-in feature to wasmer-runtime-core to enale wasm-debug so library users who won't use debug info are not affected

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2020

Build failed

  • wasmerio.wasmer

This is required because LLVM exposes its own
@MarkMcCaskey MarkMcCaskey force-pushed the feature/debug-prototype2 branch from fed7473 to a089cf5 Compare February 27, 2020 02:05
@MarkMcCaskey
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Feb 27, 2020
1212: Add support for GDB JIT debugging r=MarkMcCaskey a=MarkMcCaskey

This PR adds support for JIT debugging to Wasmer with the Cranelift backend using a fork of `wasmtime-debug`.

The motivation for this change is partially inspired by the feature in Wasmtime and the implementation is largely derived from Wasmtime's `wasmtime-debug` crate and not included in this PR.  This implementation is currently Cranelift-only (if LLVM has value tracking we can add this there too without too much effort; we'd have to do the value tracking ourselves in Singlepass and I don't have enough context to know how hard that would be) and is based on a generic fork of the `wasmtime-debug` -- which will be published and uploaded in another repo.

This PR started out implementing the [Wasm-DWARF](https://yurydelendik.github.io/webassembly-dwarf/) reading and writing with gimli but after working on it for a few days, reading a chunk of the DWARF spec, seeing that Wasmtime had solved this well, and realizing how long this would likely take, I decided that it didn't make sense to spend the engineering effort there so I made a copy of `wasmtime-debug` and removed some of the less portable Cranelift pieces (very minor changes) and all code relying on data structures from wasmtime.  The resulting crate is completely generic and would work fine with Wasmtime or any other Wasm runtime at the cost of requiring some `transmute`s or a linear pass over the debug data to reconstruct it in terms of the new types exposed by the fork.  Perhaps there's a cleaner way to handle that that I haven't considered.

The integration with the GDB JIT interface is from the LLVM examples (I don't remember if I properly attributed everything in this PR/version of the code -- I still have the other branches locally though which I'll review before shipping this) and some of the code in this PR is from Wasmtime/Cranelift source code such as the sorting of the `ebb`s in `clif_backend::resolver`.  I spent a long time debugging some subtle bugs and ended up using a few things from Wasmtime's integration with `wasmtime-debug` and some bits from `cranelift-wasm` and `cranelift-codegen`.

If there's interest from other people in working on the generic `wasmtime-debug` fork, I'm happy to get other maintainers involved and/or move it to a shared organization.

Special thanks to [Yury Delendik](https://github.com/yurydelendik) and the other `wasmtime-debug` authors for their work on Wasm debugging.  Also shout out to Cranelift for the nice API for tracking variables/data.

### TODO:
- [x] Update attributions file for LLVM, [wasm-dwarf](https://github.com/yurydelendik/wasm-dwarf), and Wasmtime/Cranelift and do another pass over code to make sure we're in compliance with the licenses from the relevant projects and have properly attributed the code used from other projects.
- [x] Adjust API of wasm-debug based on feedback
- [x] Discuss with Nick integration with LLVM
- [x] Discuss with Heyang integration with Singlepass
- [x] Adjust implementation based on feedback from team (traits modified, etc.)
- [x] Clean up some pointer wrangling code
- [x] Add opt-in feature to wasmer-runtime-core to enale wasm-debug so library users who won't use debug info are not affected

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2020

Build failed

@MarkMcCaskey
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Feb 27, 2020
1212: Add support for GDB JIT debugging r=MarkMcCaskey a=MarkMcCaskey

This PR adds support for JIT debugging to Wasmer with the Cranelift backend using a fork of `wasmtime-debug`.

The motivation for this change is partially inspired by the feature in Wasmtime and the implementation is largely derived from Wasmtime's `wasmtime-debug` crate and not included in this PR.  This implementation is currently Cranelift-only (if LLVM has value tracking we can add this there too without too much effort; we'd have to do the value tracking ourselves in Singlepass and I don't have enough context to know how hard that would be) and is based on a generic fork of the `wasmtime-debug` -- which will be published and uploaded in another repo.

This PR started out implementing the [Wasm-DWARF](https://yurydelendik.github.io/webassembly-dwarf/) reading and writing with gimli but after working on it for a few days, reading a chunk of the DWARF spec, seeing that Wasmtime had solved this well, and realizing how long this would likely take, I decided that it didn't make sense to spend the engineering effort there so I made a copy of `wasmtime-debug` and removed some of the less portable Cranelift pieces (very minor changes) and all code relying on data structures from wasmtime.  The resulting crate is completely generic and would work fine with Wasmtime or any other Wasm runtime at the cost of requiring some `transmute`s or a linear pass over the debug data to reconstruct it in terms of the new types exposed by the fork.  Perhaps there's a cleaner way to handle that that I haven't considered.

The integration with the GDB JIT interface is from the LLVM examples (I don't remember if I properly attributed everything in this PR/version of the code -- I still have the other branches locally though which I'll review before shipping this) and some of the code in this PR is from Wasmtime/Cranelift source code such as the sorting of the `ebb`s in `clif_backend::resolver`.  I spent a long time debugging some subtle bugs and ended up using a few things from Wasmtime's integration with `wasmtime-debug` and some bits from `cranelift-wasm` and `cranelift-codegen`.

If there's interest from other people in working on the generic `wasmtime-debug` fork, I'm happy to get other maintainers involved and/or move it to a shared organization.

Special thanks to [Yury Delendik](https://github.com/yurydelendik) and the other `wasmtime-debug` authors for their work on Wasm debugging.  Also shout out to Cranelift for the nice API for tracking variables/data.

### TODO:
- [x] Update attributions file for LLVM, [wasm-dwarf](https://github.com/yurydelendik/wasm-dwarf), and Wasmtime/Cranelift and do another pass over code to make sure we're in compliance with the licenses from the relevant projects and have properly attributed the code used from other projects.
- [x] Adjust API of wasm-debug based on feedback
- [x] Discuss with Nick integration with LLVM
- [x] Discuss with Heyang integration with Singlepass
- [x] Adjust implementation based on feedback from team (traits modified, etc.)
- [x] Clean up some pointer wrangling code
- [x] Add opt-in feature to wasmer-runtime-core to enale wasm-debug so library users who won't use debug info are not affected

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2020

Build failed

  • wasmerio.wasmer

@MarkMcCaskey
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Feb 27, 2020
1212: Add support for GDB JIT debugging r=MarkMcCaskey a=MarkMcCaskey

This PR adds support for JIT debugging to Wasmer with the Cranelift backend using a fork of `wasmtime-debug`.

The motivation for this change is partially inspired by the feature in Wasmtime and the implementation is largely derived from Wasmtime's `wasmtime-debug` crate and not included in this PR.  This implementation is currently Cranelift-only (if LLVM has value tracking we can add this there too without too much effort; we'd have to do the value tracking ourselves in Singlepass and I don't have enough context to know how hard that would be) and is based on a generic fork of the `wasmtime-debug` -- which will be published and uploaded in another repo.

This PR started out implementing the [Wasm-DWARF](https://yurydelendik.github.io/webassembly-dwarf/) reading and writing with gimli but after working on it for a few days, reading a chunk of the DWARF spec, seeing that Wasmtime had solved this well, and realizing how long this would likely take, I decided that it didn't make sense to spend the engineering effort there so I made a copy of `wasmtime-debug` and removed some of the less portable Cranelift pieces (very minor changes) and all code relying on data structures from wasmtime.  The resulting crate is completely generic and would work fine with Wasmtime or any other Wasm runtime at the cost of requiring some `transmute`s or a linear pass over the debug data to reconstruct it in terms of the new types exposed by the fork.  Perhaps there's a cleaner way to handle that that I haven't considered.

The integration with the GDB JIT interface is from the LLVM examples (I don't remember if I properly attributed everything in this PR/version of the code -- I still have the other branches locally though which I'll review before shipping this) and some of the code in this PR is from Wasmtime/Cranelift source code such as the sorting of the `ebb`s in `clif_backend::resolver`.  I spent a long time debugging some subtle bugs and ended up using a few things from Wasmtime's integration with `wasmtime-debug` and some bits from `cranelift-wasm` and `cranelift-codegen`.

If there's interest from other people in working on the generic `wasmtime-debug` fork, I'm happy to get other maintainers involved and/or move it to a shared organization.

Special thanks to [Yury Delendik](https://github.com/yurydelendik) and the other `wasmtime-debug` authors for their work on Wasm debugging.  Also shout out to Cranelift for the nice API for tracking variables/data.

### TODO:
- [x] Update attributions file for LLVM, [wasm-dwarf](https://github.com/yurydelendik/wasm-dwarf), and Wasmtime/Cranelift and do another pass over code to make sure we're in compliance with the licenses from the relevant projects and have properly attributed the code used from other projects.
- [x] Adjust API of wasm-debug based on feedback
- [x] Discuss with Nick integration with LLVM
- [x] Discuss with Heyang integration with Singlepass
- [x] Adjust implementation based on feedback from team (traits modified, etc.)
- [x] Clean up some pointer wrangling code
- [x] Add opt-in feature to wasmer-runtime-core to enale wasm-debug so library users who won't use debug info are not affected

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2020

Build failed

@MarkMcCaskey
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Feb 27, 2020
1212: Add support for GDB JIT debugging r=MarkMcCaskey a=MarkMcCaskey

This PR adds support for JIT debugging to Wasmer with the Cranelift backend using a fork of `wasmtime-debug`.

The motivation for this change is partially inspired by the feature in Wasmtime and the implementation is largely derived from Wasmtime's `wasmtime-debug` crate and not included in this PR.  This implementation is currently Cranelift-only (if LLVM has value tracking we can add this there too without too much effort; we'd have to do the value tracking ourselves in Singlepass and I don't have enough context to know how hard that would be) and is based on a generic fork of the `wasmtime-debug` -- which will be published and uploaded in another repo.

This PR started out implementing the [Wasm-DWARF](https://yurydelendik.github.io/webassembly-dwarf/) reading and writing with gimli but after working on it for a few days, reading a chunk of the DWARF spec, seeing that Wasmtime had solved this well, and realizing how long this would likely take, I decided that it didn't make sense to spend the engineering effort there so I made a copy of `wasmtime-debug` and removed some of the less portable Cranelift pieces (very minor changes) and all code relying on data structures from wasmtime.  The resulting crate is completely generic and would work fine with Wasmtime or any other Wasm runtime at the cost of requiring some `transmute`s or a linear pass over the debug data to reconstruct it in terms of the new types exposed by the fork.  Perhaps there's a cleaner way to handle that that I haven't considered.

The integration with the GDB JIT interface is from the LLVM examples (I don't remember if I properly attributed everything in this PR/version of the code -- I still have the other branches locally though which I'll review before shipping this) and some of the code in this PR is from Wasmtime/Cranelift source code such as the sorting of the `ebb`s in `clif_backend::resolver`.  I spent a long time debugging some subtle bugs and ended up using a few things from Wasmtime's integration with `wasmtime-debug` and some bits from `cranelift-wasm` and `cranelift-codegen`.

If there's interest from other people in working on the generic `wasmtime-debug` fork, I'm happy to get other maintainers involved and/or move it to a shared organization.

Special thanks to [Yury Delendik](https://github.com/yurydelendik) and the other `wasmtime-debug` authors for their work on Wasm debugging.  Also shout out to Cranelift for the nice API for tracking variables/data.

### TODO:
- [x] Update attributions file for LLVM, [wasm-dwarf](https://github.com/yurydelendik/wasm-dwarf), and Wasmtime/Cranelift and do another pass over code to make sure we're in compliance with the licenses from the relevant projects and have properly attributed the code used from other projects.
- [x] Adjust API of wasm-debug based on feedback
- [x] Discuss with Nick integration with LLVM
- [x] Discuss with Heyang integration with Singlepass
- [x] Adjust implementation based on feedback from team (traits modified, etc.)
- [x] Clean up some pointer wrangling code
- [x] Add opt-in feature to wasmer-runtime-core to enale wasm-debug so library users who won't use debug info are not affected

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <[email protected]>
Co-authored-by: Mark McCaskey <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 27, 2020

Build failed

  • wasmerio.wasmer

@MarkMcCaskey
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 27, 2020

Build succeeded

@bors bors bot merged commit a13a897 into master Feb 27, 2020
@bors bors bot deleted the feature/debug-prototype2 branch February 27, 2020 22:41
@webmaster128
Copy link
Contributor

How should I interprete the new source_loc? Is it the 0-based index of the wasm blob in bytes?

We're implementing a

impl FunctionMiddleware for DeterministicMiddleware {
    type Error = CompileError;

    fn feed_event<'a, 'b: 'a>(
        &mut self,
        event: Event<'a, 'b>,
        _module_info: &ModuleInfo,
        sink: &mut EventSink<'a, 'b>,
        source_loc: u32,
    ) -> Result<(), Self::Error> {
        match event {
            Event::Wasm(op) => parse_wasm_opcode(op)?,
            Event::WasmOwned(ref op) => parse_wasm_opcode(op)?,
            _ => (),
        };

        sink.push(event);
        Ok(())
    }
}

and could pass the new value into the error message if that makes sense.

@MarkMcCaskey
Copy link
Contributor Author

@webmaster128 source_locs should be forwarded without modification for most middleware. Yeah, source_locs are offsets into the Wasm binary from the start of the code section if I remember correctly -- though I don't know if that's subject to change in the future. From the perspective of most of the code touching them, they can just be treated as a black-box. You could use them in error messages but the semantics of the number may break in the future if the Wasm DWARF standard changes, so I would recommend not relying on it for anything critical if you can avoid it!

@webmaster128
Copy link
Contributor

Thanks a lot, @MarkMcCaskey, that's very helpful. I'll ignore the value then. I've no place to forward it to.

@MarkMcCaskey
Copy link
Contributor Author

Sure! We'll update the CHANGELOG.md with any breaking changes, so if the Wasm DWARF standard does change, we'd document it there. So if you check that when updating Wasmer, you shouldn't have any bad surprises like the meaning of source_loc changing (if you do rely on it)!

@ouiliame
Copy link

ouiliame commented May 3, 2023

Hi, was this feature removed? I can't seem to trace why it was deprecated, was it due to lack of maintenance? Seems like a pretty important feature to leave out.

@ptitSeb
Copy link
Contributor

ptitSeb commented May 3, 2023

It was removed, but that looks like a nice feature, so I'll check if it can be added back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 lib-cli About wasmer-cli 📦 lib-compiler-cranelift About wasmer-compiler-cranelift 📦 lib-deprecated About the deprecated crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants